[PluggableHID] API simplification proposal #3840

cmaglie · 2015-09-22T15:15:43Z

Since the PluggableHID is not yet officially released maybe we could make some makeup on the API?
This is a breaking change for the early adopters of the Pluggable HID, but if we decide to merge it better doing it now that the library is not published.

The most ugly part for me is how the HID "modules" are defined, for example:

Keyboard_::Keyboard_(void) 
{
    static HID_Descriptor cb = {
        .length = sizeof(_hidReportDescriptor),
        .descriptor = _hidReportDescriptor,
    };
    static HIDDescriptorListNode node(&cb);
    HID.AppendDescriptor(&node);
}

IMHO the HID_Descriptor is a useless level of abstraction and can be eliminated.
After this PR the initialization becomes:

Mouse_::Mouse_(void) : _buttons(0)
{
    static HIDDescriptorListNode node(_hidReportDescriptor, sizeof(_hidReportDescriptor));
    HID.AppendDescriptor(&node);
}

This give us also a small gain in performance and in flash and ram usage as well.

/cc @NicoHood @facchinm @matthijskooijman

In particular HIDDescriptorListNode.cb has been renamed to HIDDescriptorListNode.descriptor because it contains decriptor data and not callbacks. Moreover the HID_Descriptor.descriptor field has been renamed to HID_Descriptor.data so the structure has now two fields length and data. typedef struct __attribute__((packed)) { uint16_t length; const void* data; } HID_Descriptor; class HIDDescriptorListNode { public: HIDDescriptorListNode *next = NULL; const HID_Descriptor *descriptor; HIDDescriptorListNode(const HID_Descriptor *d) : descriptor(d) { } }; This imply a change in the use of the node from: node->cb->lenght node->cd->descriptor to node->descriptor->length node->descriptor->data

This simplifies the object model and produce a small gain in code size and performance.

NicoHood · 2015-09-22T15:53:58Z

If you really want to change the API I recommend you to do it right and add all the functions I've added in my extended version. Maybe not all, but the key features. Then people can create cool stuff even with the Arduino Core and not only with my patched HID-Project.

I also suggest to make the HID Interface itself "multi pluggable". Right now you have a single HID Interface for all the HID Devices. But you can also create an Interface her HID Device. This avoids lots of problems and makes it easier to add a "non workaround" boot keyboard and an option for things like raw HID. Also multiple gamepads would be then possible. Gamepads on a multireport just dont work, dont try.

I was currently working on raw HID and I got it setup properly under linux. Windows wont work right know, I am pretty sure. But I can make this work if I want to.

If you have more time for applying changes I'd like to help out here. But I need more time to some up with a clever and useful plan. If we change this over and over again, noone is happy. If we think clearly about it, we probably can make this API pretty fancy. And the way the pluggable HID works right now is pretty impressive I think.

Thinking of:
HID>HID Interface>HID Device>Single Report || Multi Report>APIs(Keyboard etc)
You can add multiple HID Interfaces on multiple endpoints. All work similar, so we can dedupe this code. Then you have an HID Device with 2 different implementations (childs): with a single report (no report ID) and multireport (current solution). Then you can attach the APIs to a single or multireport. As said this is not final, just an idea I try to come up with. Then you can create almost any HID device.

How do you feel about this?

NicoHood · 2015-09-22T16:28:01Z

Thinking about this some more it would probably make sense to build the pluggable USB the same way I improved the pluggable HID. If you create a mean class USBInterface that every other Interface can inherit (HID, Serial) its way simpler to code multiple HID Interfaces. Because then you can add a this pointer automatically via inheritation etc. This also gives us the option to avoid static functions inside or outside the class and probably saves a lot of doubled pointers (ram + flash again).

Once this is done it would be possible to create a general HID Interface class with a similar API that can be used for different HID Devices (if you want each device on a separate endpoint. Reasons why described in the post above).

This requires even more changes but helps to make the Pluggable USB as best as possible. I think I am not able to improve the HID unless the USB itself is patched (there we have this problem again).

Comments?

ArduinoBot · 2015-09-22T16:33:07Z

Build successful. Please test this code using one of the following:
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-3840-BUILD-399-linux32.tar.xz
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-3840-BUILD-399-linux64.tar.xz
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-3840-BUILD-399-windows.zip
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-3840-BUILD-399-macosx.zip

cmaglie · 2015-09-25T14:49:35Z

Hi @NicoHood

well, when I said "make some makeup on the API" I meant "let's see if we can do some small adjustments on the API" before the release :-)

With this PR my aim is just to remove the ugliness of the HID_Report structure because it seems more like a refactoring leftover than something put there on purpose and this is confirmed also by the presence, on the same HID.h, of another struct called HIDReport (without the underscore!).

I understand your goodwill to change the API as best as possible but, please, consider also release timings in the mix: making substantial changes in the HID API now implies doing another round of porting/coding/testing on various OS (winXP/7/8/10, Mac 10.7/8/9/10, Linux 32/64) and various platform (AVR, SAM, SAMD) that may take long time.
I don't want to release a sub-optimal API, but on the other hand I don't want to postpone forever waiting for perfection.

So to move forward my question is: all the missing feature you mentioned may be added, not now, but in a second release of the HID library? Adding new features in a second release without breaking the current API is generally fine, so my question is intended to understand if all the new features you have in mind could be added in a second time without breaking the current API.

NicoHood · 2015-09-25T14:57:57Z

All features I have in mind are not possible with the current API. At least for HID. I can still write my own HID API, no problem. But it would be easier if that would be in the Arduino code.

Maybe I'd also change some stuff in the USB API too, (similar flexible changes as in the HID). The USB API is more important than the HID, because I always can replace the HID API with my own. But you are right, we cant postpone this forever. I think I will go with my own version and Arduino with its own.

Its all about backwards compatibility, I know. That holds back the progress.

I have to see what is most annoying in the USB API that needs to be changed. I think I will hold back my HID changes since I am still developing much on this and dont have a clear Idea whats the best solution for everything.

When is the next release planned? So I have some time to look at the USB Core or just give up since its not enough time.

NicoHood · 2015-09-25T14:59:55Z

The PR itself looks good to me though. That should be changed at minimum ;)

NicoHood · 2015-09-25T15:01:23Z

hardware/arduino/sam/libraries/HID/HID.h

-  const HID_Descriptor * cb;
-  HIDDescriptorListNode(const HID_Descriptor *ncb) {cb = ncb;}
+  HIDDescriptorListNode(const void *d, uint16_t l) : data(d), length(l) { }
+  uint8_t length;


uint16_t? Also this could be const (avr as well)?

uint16_t?

It should allow for descriptors >256.

Also this could be const (avr as well)?

Mbah, does it really matter? l is passed by copy so the method can't change the "original" parameter in any way.

yes. but its uint8_t. look ;D

Its not about changing permissions, its about code style, debugging and compiler optimization.

Ahhh, right good catch! ;-)

NicoHood · 2015-09-26T16:13:03Z

Yeah I checked that again.

It would be nice to pack this stuff inside a real class that can be inherited:
https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/PluggableUSB.h#L28-L49

This way we can generalize and USB Device and inherit it further. For example I can inherit the usb device, create global HID class and then specify different HID Device classes (keyboard etc) with different endpoints.

Similar to this:
https://github.com/NicoHood/HID/blob/dev_2_4/src/PluggableHID/HIDDevice.h
https://github.com/NicoHood/HID/blob/dev_2_4/src/ImprovedKeyboard.h#L53

At the moment I can only duplicate the code with a lot of messy stuff.

See #3840 (comment)

Teiwaz83 · 2015-09-29T02:19:02Z

As one of the "early adopters" of PluggableHID, I'd rather have to do a bit of rework now than deal with a mess and/or limited functionality later. Here's some minor sore spots for me as I've been working with it:

It might be smart to rename (from HID.h)

HID_ENDPOINT_INT
HID_INTERFACE

These guys used to be constants defined by macros before PluggableUSB. Now they just look like they are. I found this very confusing when reading the constructor for HID and HID_GetInterface, as it looked at first brush like there was a weird circular dependency between the constructor and HID_GetInterface. It all makes sense when you dig into it, but starting out with wrong assumptions about what these are doesn't help. Also, it leads to painful things like this:

int HID_::HID_GetInterface(u8* interfaceNum)
{
    interfaceNum[0] += 1;   // uses 1
    _hidInterface.hid =
#if defined(USE_BOOT_KEYBOARD_PROTOCOL)
        D_INTERFACE(HID_INTERFACE,1,3,1,1);
#elif defined(USE_BOOT_MOUSE_PROTOCOL)
        D_INTERFACE(HID_INTERFACE,1,3,1,2);
#else
        D_INTERFACE(HID_INTERFACE,1,3,0,0);
#endif
    _hidInterface.desc = D_HIDREPORT(sizeof_hidReportDescriptor);
    _hidInterface.in = D_ENDPOINT(USB_ENDPOINT_IN (HID_ENDPOINT_INT),USB_ENDPOINT_TYPE_INTERRUPT,USB_EP_SIZE,0x01);
    return USB_SendControl(0,&_hidInterface,sizeof(_hidInterface));
}

Half of the constant/macro-looking things in there are actually variables. More than that, they're variables which don't actually have their values set yet at the only point this function is referenced in the HID class.

Also, I'd suggest doing a quick pass on the various descriptor variable names and structure names:

typedef struct
{
  u8 len;     // 9
  u8 dtype;   // 0x21
  u8 addr;
  u8  versionL; // 0x101
  u8  versionH; // 0x101
  u8  country;
  u8  desctype; // 0x22 report
  u8  descLenL;
  u8  descLenH;
} HIDDescDescriptor;

typedef struct 
{
  InterfaceDescriptor     hid;
  HIDDescDescriptor     desc;
  EndpointDescriptor      in;
} HIDDescriptor;

The... HID Descriptor contains a HID... Descriptor Descriptor? So the HIDDescriptor _hidInterface contains a HID Descriptor Descriptor called desc. Not to be confused with the InterfaceDescriptor contained by the HID Descriptor called hid. Not the HID Descriptor, mind you, as we already established, the HIDDescriptor is called _hidInterface. Headache.

I find myself having to constantly refer back to the USB spec, trying to figure out what various almost identically named macros and structures are by counting the number of bytes in them. I'd suggest making the names of these structures and their members to precisely match their equivalents in the USB spec for easy comparison and less ambiguity. (Hell, I'd also stick in comments with hyperlinks to the relevant parts of the USB spec. But I assume that'd be frowned upon here. Go team JavaDoc!)

facchinm · 2015-09-29T07:48:05Z

Hi @Teiwaz83 and @NicoHood , you are obviously pointing in the right direction.
Keep in mind that the whole Pluggable* effort was not a rewrite but a porting of existing code, so no struct was rationalized and existing macros which needed to be dynamic kept the same name (my fault).

Anyway, thanks for sharing your ideas and improvements, the whole HID library needs some love now but it's quite easier than before since it's outside the core 😄

See arduino#3840 (comment)

NicoHood · 2015-09-29T15:44:32Z

First progress can be found here:
https://github.com/arduino/Arduino/compare/master...NicoHood:pluggableUSB?expand=1

I now try to get an actual HID device running. Seems like the change (for the other APIs like HID) is not that huge. And this gives us a whole new more flexible options.

cmaglie · 2015-09-29T16:13:40Z

Hi Nico,

I'm also trying some simplification, most looks similar to yours, have a look here:

https://github.com/arduino/Arduino/compare/master...cmaglie:phid-class?expand=1

It seems that we started at the same time, so let's try to converge the efforts!

See arduino/Arduino#3840 (comment)

cmaglie added 2 commits September 22, 2015 16:49

HID: merged HID_Descriptor into HIDDescriptorListNode

401c4f0

This simplifies the object model and produce a small gain in code size and performance.

cmaglie added the Library: HID The HID Arduino library label Sep 22, 2015

cmaglie self-assigned this Sep 22, 2015

NicoHood mentioned this pull request Sep 22, 2015

Pluggable USB does not check maximum endpoint number #3842

Closed

NicoHood reviewed Sep 25, 2015
View reviewed changes

NicoHood mentioned this pull request Sep 28, 2015

PUSBCallbacks - is it necessary to require static functions? (PluggableUSB in 1.6.6) #3874

Closed

cmaglie merged commit 401c4f0 into arduino:master Sep 28, 2015

cmaglie deleted the pluggable_hid_impr branch September 28, 2015 10:07

cmaglie added this to the Release 1.6.6 milestone Sep 28, 2015

cmaglie added a commit that referenced this pull request Sep 28, 2015

[HID] added 'const' qualifier in HID initialization

209b079

See #3840 (comment)

sandeepmistry pushed a commit to sandeepmistry/Arduino that referenced this pull request Sep 29, 2015

[HID] added 'const' qualifier in HID initialization

07798dd

See arduino#3840 (comment)

cmaglie mentioned this pull request Sep 29, 2015

[PluggableHID] 2nd round of API improvement (WIP) #3896

Merged

facchinm pushed a commit to arduino/ArduinoCore-sam that referenced this pull request Jan 19, 2017

[HID] added 'const' qualifier in HID initialization

651519d

See arduino/Arduino#3840 (comment)

neu-rah pushed a commit to neu-rah/VirtualPins that referenced this pull request Apr 10, 2017

[HID] added 'const' qualifier in HID initialization

1a14195

See arduino/Arduino#3840 (comment)

facchinm pushed a commit to arduino/ArduinoCore-avr that referenced this pull request Sep 20, 2017

[HID] added 'const' qualifier in HID initialization

937c859

See arduino/Arduino#3840 (comment)

facchinm pushed a commit to arduino/ArduinoCore-avr that referenced this pull request Sep 20, 2017

[HID] added 'const' qualifier in HID initialization

6a05a26

See arduino/Arduino#3840 (comment)

facchinm pushed a commit to arduino/ArduinoCore-avr that referenced this pull request Sep 20, 2017

[HID] added 'const' qualifier in HID initialization

40a2b1c

See arduino/Arduino#3840 (comment)

facchinm pushed a commit to arduino/ArduinoCore-avr that referenced this pull request Sep 20, 2017

[HID] added 'const' qualifier in HID initialization

bf5eaa7

See arduino/Arduino#3840 (comment)

facchinm pushed a commit to arduino/ArduinoCore-avr that referenced this pull request Sep 20, 2017

[HID] added 'const' qualifier in HID initialization

37e2914

See arduino/Arduino#3840 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PluggableHID] API simplification proposal #3840

[PluggableHID] API simplification proposal #3840

cmaglie commented Sep 22, 2015

NicoHood commented Sep 22, 2015

NicoHood commented Sep 22, 2015

ArduinoBot commented Sep 22, 2015

cmaglie commented Sep 25, 2015

NicoHood commented Sep 25, 2015

NicoHood commented Sep 25, 2015

NicoHood Sep 25, 2015

cmaglie Sep 28, 2015

NicoHood Sep 28, 2015

cmaglie Sep 28, 2015

NicoHood commented Sep 26, 2015

Teiwaz83 commented Sep 29, 2015

facchinm commented Sep 29, 2015

NicoHood commented Sep 29, 2015

cmaglie commented Sep 29, 2015

[PluggableHID] API simplification proposal #3840

[PluggableHID] API simplification proposal #3840

Conversation

cmaglie commented Sep 22, 2015

NicoHood commented Sep 22, 2015

NicoHood commented Sep 22, 2015

ArduinoBot commented Sep 22, 2015

cmaglie commented Sep 25, 2015

NicoHood commented Sep 25, 2015

NicoHood commented Sep 25, 2015

NicoHood Sep 25, 2015

Choose a reason for hiding this comment

cmaglie Sep 28, 2015

Choose a reason for hiding this comment

NicoHood Sep 28, 2015

Choose a reason for hiding this comment

cmaglie Sep 28, 2015

Choose a reason for hiding this comment

NicoHood commented Sep 26, 2015

Teiwaz83 commented Sep 29, 2015

facchinm commented Sep 29, 2015

NicoHood commented Sep 29, 2015

cmaglie commented Sep 29, 2015